This requires Kibana to know the _ID of the logout request crafted by ES and sent to the idP. The current SamlLogoutResponse does not give this information in a way that Kibana can easily access.

We could either change SamlLogoutResponse to include a separate field of _ID similar to what we do with SamlPrepareAuthenticationResponse. Or we could simply skip this check?

The spec (4.4.4.2) doesn't talk about validating the InResponseTo parameter, but I've seen other libraries / impementation do that. In all honesty SAML SLO is such a wild wild west, everyone does pretty much what they feel like :/

Given that it's not dictated by the specification and that our API has a binary response of "The Logout Response is fine" vs "the Logout Response is not fine" which cannot be abused somehow ( ie. open redirect etc) I think it would be fine to not validate but then again the implementation of doing it is already there so why not ? ( Kibana would need to make changes either way )

It's straightforward to add a new requestId field to the SamlLogoutResponse ES produces (via the /_security/saml/logout API). Given it is a change to an existing API, we may need to communicate to the Kibana team earlier on. @legrego Could you please comment on this? The requestId value is then required to be passed back to ES from Kibana to verify the LogoutResponse from the idP.

Given it is a change to an existing API, we may need to communicate to the Kibana team earlier on

In general, adding additional fields to our responses is not considered to be a breaking change so I believe Kibana will be fine with it even if we don't sync merging support for this.

The requestId value is then required to be passed back to ES from Kibana to verify the LogoutResponse from the idP.

Kibana does that already for the request ID of the authentication response and send this back to ES in the _security/saml/authenticate API so I believe it would be fine to do here too.

Thanks @jkakavas I'll make the change accordingly.

In general, adding additional fields to our responses is not considered to be a breaking change so I believe Kibana will be fine with it even if we don't sync merging support for this.

That's correct.

Kibana does that already for the request ID of the authentication response and send this back to ES in the _security/saml/authenticate API so I believe it would be fine to do here too.

The main difference here is that request ID returned from _security/saml/prepare is stored in the cookie. But during logout flow we used to clear cookie when user is redirected to IdP with LogoutRequest. Having said that, we can change that, we'll just need to carefully fix all the remaining places that still treat any cookie as a sign of authenticated user.

Since the specification doesn't explicitly call this out AND the outcome is in any case a no-op, I would be ok with not validating this. Especially since it sounds possible that the risk of introducing a bug on Kibana's side by changing this behavior is not minimal. On the other hand, treating "any cookie as a sign of authenticated user." is something worth fixing regardless of that change so if this is an extra driver for that change, let's do it !

The response MUST be signed (4.4.4.2) so we should fail if it is not signed.

The spec says "Must authenticate itself either by signing or a binding specific mechansim". While the HTTP-POST LogoutResponse is signed, the LogoutResponse from HTTP-Redirect binding is not. So I would prefer it to be optional.

or a binding specific mechanism

this alludes to the artifact binding that is not used/supported here

. While the HTTP-POST LogoutResponse is signed, the LogoutResponse from HTTP-Redirect binding is not.

Why are you stating that as a fact ? It can very well be signed and according to the specification it MUST be.

Why are you stating that as a fact ?

It's not an universal fact. I was trying to avoid explicitly mentioning a popular idP's name. Sorry about the confusion. If we mandate signature verification and Kibana starts to redirect the response to ES, the verification will fail since it does not have signature.

In addition, including signature in HTTP-Redirect request may risk hitting the (practical) limit of URL length. I guess this is probably the reason why it is not signed by the aforementioned idP.

If we mandate signature verification and Kibana starts to redirect the response to ES, the verification will fail since it does not have signature.

It all boils down to how permissive we want to be with things that go against the specification. If a popular IDP doesn't adhere to the specification, do we happily consume what they are sending us or do we not ? And if another popular IDP comes along and doesn't adhere to the standards in a different way, do we happily accept that too? Where do we draw the line? I believe that drawing the line at what the specification says is easier to argue about than drawing the line in an arbitrary leniency we deem acceptable at some point.

In addition, including signature in HTTP-Redirect request may risk hitting the (practical) limit of URL length. I guess this is probably the reason why it is not signed by the aforementioned idP

I doubt that. SAML Logout Response messages are quite small ( comparable to authentication requests that also use the HTTP Redirect binding and are signed )

First of all, thanks for indulging me with the discussions. I really appreciate it. It helps me understanding the issue better ❤️

It is when it is configured to be

Thanks for pointing this out. I completely ignored this part when setting up saml locally. But isn't it actually an example that we do allow unsigned LogoutResponse (the difference being we are the sender in this case)?

SAML with authentication requests ( using the HTTP Redirect binding) being signed has been out there for 15 years now, this is heavily supported and used

This is a good point as well. Given authn request can be signed and use HTTP Redirect, I agree there will be no issue for LogoutResponse.

I don't think this is true.

You are right that it is not "optional". The word used in the spec 6.4.2 is "SHOULD". It is stronger than optional, but not MUST. Our implementation for this part treats it as optional:

But isn't it actually an example that we do allow unsigned LogoutResponse (the difference being we are the sender in this case)?

I don't remember the discussions we had while implementing this , I can assume we made a decision that interoperability is more important than specification adherence at that time. We can revisit that decision too :)

You are right that it is not "optional". The word used in the spec 6.4.2 is "SHOULD".

We only support the web browser single sign on profile, so 4.1 in that spec, see 4.1.3.5

Our implementation for this part treats it as optional:

It -hopefully- doesn't!!! A SAML response contains SAML assertions. What we do support is either the response or the assertion is signed :) There must be an integrity protection implemented, otherwise it would be trivial for attackers to bypass authentication

We only support the web browser single sign on profile, so 4.1 in that spec, see 4.1.3.5

You are right. I read the wrong section 🤦

What we do support is either the response or the assertion is signed

Yes that's what I figured as well. There is no risk in this part.

I think the only thing that bothers me is this "popular idP". The original issue #43264 is created with it as an example of why we consider to support HTTP-POST LogoutResponse. So it feels rather unsatifying if we fix one binding and breaks the other (the current working one).

The code change itself either way is trivial. The decision making part is more important. I feel we could talk about this over sync time.

I think the only thing that bothers me is this "popular idP".

I don't think we need to make changes that satisfy any "popular IDP", we should make changes that make sense for the users and are correct. What's more, I see no indication whatsoever that any IDP doesn't support signed Logout Responses so I'm not sure what we would be actually breaking with this.

I think we should not be lenient here for no reason, but happy to discuss this more on the sync.

Sync update: we decided to conform to the spec and perform full validation. Caller of the API can decide what to do with validation errors if any. Thanks for the discussion.

I don't think we need handle BWC here since this response never goes across nodes. The only consumer is Kibana (or other external system that integrates with ES).

I would prefer if we have different fields for this depending on the binding

I was intentionally trying to make it the same to handle both bindings. The current approach is to have a single POST endpoint that Kibana can call for both HTTP-Redirect and HTTP-Post bindings from the idP, i.e. Kibana acts as a multiplexer here. So what ES receives is an uniform POST message. The handler (SamlLogoutResponseHandler) is able to tell whether the paylod (content) itself is signed and proceeds as HTTP-Redirect. If it is not, it proceeds as HTTP-POST and requires the XML to be signed, which is more or less similar to what SamlAuthenticator does.

I can be convinced to have two separate endpoints, one for HTTP-Redirect and one for HTTP-POST. If we do that, we can even have two separate Request classes as well. But if we choose to have a single endpoint, I'd prefer to also not differentiate it at the Request object level.

Whether we should have one or two end-points also depends on what Kibana can do (@azasypkin). My thinking is that Kibana will have to prepare the requset to ES anyhow, i.e. it has to add the realm and requestId information. So Kibana might as well just act as the multiplexer and prepare an uniform message.

After reading your comment below. I am convinced to have separate fields at request level, queryString and content for HTTP-Redirect and HTTP-POST, respectively. The names are chosen to be consistent with RestSamlInvalidateSessionAction and RestSamlAuthenticateAction.

The SamlCompleteLogoutRequest will validate that the two fields cannot both be null nor both be set. It also has convenient methods, getPayload and isHttpRedirect, so the information is clear at handler level. Thanks.

I don't think we need the assertionConsumerServiceURL in this case. IIUC ( @azasypkin can keep me honest ) , kibana's current versions can be made to always send the realm name in the request which is enough for us to get the realm by name

Correct, starting from 7.7 we store realm returned from the _security/saml/authenticate in the cookie/session and we'll keep it when store Logout Request ID there as well.

Thanks. Good to know that. Removed.

Leave this in a method as it was and just name it differently ? WDYT ?

Sure, named it checkSubjectInResponseTo. Let me know if you have a better name.

I think I don't see the value in splitting up SamlObjectHandler and SamlResponseHandler. Can you elaborate ? It also doesn't help that all the objects that are responses for SAML , are passed in a request to elasticsearch and that makes naming hard but I'm equally happy with SamlResponseHandler and SamlRequestHandler

This is mainly because we do have a real Request object from idP, i.e.SamlInvalidateSessionRequest. Its associated handler is SamlLogoutRequestHandler, which is a subclass of SamlObjectHandler. In another word, SamlObjectHandler is the base class for both Request and Response handlers. So I did not merge SamlResponseHandler into SamlObjectHandler.

Also I did a few refactoring which is likely made the changes hard to track, here is a summary:

• SamlObjectHandler was renamed from SamlRequestHandler (It seems to be a good idea to me since its the base for both Request and Response handlers. But I can revert it if it does not sound right to you).
• SamlResponseHandler is extracted from SamlAuthenticator since many code can be reused by the new SamlLogoutResponseHandler

The class hierarchy is as follows:

SamlObjectHandler
├── SamlLogoutRequestHandler
└── SamlResponseHandler
    ├── SamlAuthenticator
    └── SamlLogoutResponseHandler

I assume these are carried over as is from SamlAuthenticatorTests so I'm fine with these, let me know if you've made any significant changes somehow

You are right. They are split with the IDE refactoring tool. So nothing should be really different.

related to the discussion above, this is a case where we don't want to fail to parse the SAML message because we will try to parse it as if it came from the HTTP POST binding. We want to fail this because it is an unsigned request that came via the HTTP-Redirect binding and we disallow that.

I think I see your point here. I'll move away from auto-detect and have two separate fields at request level.

I would very much prefer that we are explicit here and not try to deduce and imply checks. I think we need two parameters for the REST layer, one for the body of a response that comes via the HTTP-POST binding and a different one for the query string of a response that comes via the HTTP-Redirect binding. Then we can be very explicit here with regards to what we expect to have and what we need to process/check/validate

See above.

Again, assuming that you didn't change much when you copied this over ( apart from introducing samlMessageParameterName ) , please do flag if otherwise ! ( diffs are not that helpful in spotting this kind of things :) )

Good spot! Indeed this is the only significant change as part of the refactoring. Next time I'll compile the list of changes before hand to save you some time. Sorry about that. Here is a list for rest of the changes (they are all very minor):

• method visibility from private to protect due to superclass extraction.
  • This includes: SamlResponseHandler#isSuccess, SamlResponseHandler#getStatusCodeMessage, SamlObjectHandler#decodeBase64, SamlObjectHandler#inflate, and SamlObjectHandler#parseQueryStringAndValidateSignature.
• Reverse the order of String equals compare in SamlResponseHandler#isSuccess.
  • Changed from status.getStatusCode().getValue().equals(StatusCode.SUCCESS) to StatusCode.SUCCESS.equals(status.getStatusCode().getValue()).

It's not the URL that is signed, but rather the - encoded - SAML response.

You are right that it is not the entire URL that gets signed. But it is not just the encoded SAMLResponse either. It is rather a string taking the form of SAMLResponse=xxx&RelayState=xxx&SigAlg=xx. How about changing it to "Query string is not signed, but is required for HTTP-Redirect binding"?

The most important part here is for the user/administrator to realize what is wrong and why this failed. I think that "The SAML logout response is not signed" does that in a better way than "the URL is not signed" or "The query string is not signed" but I won't insist too much :)

It reads strange that we "parse the query string" when payload might very well be the body of HTTP POST

Reading through the tests below, it looks like there is an implicit assumption that even when HTTP POST is used, we will receive SAMLResponse=<Base64Endoded saml logout response here> from Kibana and thus we could "parse" this as if it was a query string. I think we should be explicit and

a) Declare that we expect kibana(or any caller) to pass the value of the SAMLResponse POST parameter
b) Base64 decode this on the RestSamlLogoutAction
c) Operate on the raw bytes after that

similar to what we do for the SamlAuthenticateAction's parameter named content

The payload/data returned from a SAML idP with HTTP-Post binding uses application/x-www-form-urlencoded encoding, which takes the form of SAMLResponse=<base64>&RelayState=xxxx. So it can indeed be processed as if it is a query string.

But I think it is good point that we should keep this consistent with how SAMLReseponse is handled by SamlAuthenticationAction, for which I think Kibana performs the action of extracting the value of SAMLResponse parameter and passes it to ES so that we do not have to parse it (as query parameter) again. I will make the change and add a note in the code to clarify the expectation for Kibana.

Given that I had 4 comments in 10 lines ( Woke up picky today, apologies 🙏 ) think we could do this is a simpler way, something like

final Element root;
if (httpRedirect) {
    logger.debug("Process SAML LogoutResponse with HTTP-Redirect binding");
    final ParsedQueryString parsed = parseQueryStringAndValidateSignature(payload, "SAMLResponse");
    if (parsed.hasSignature == false){
        throw samlException("SAML LogoutResponse messages must be signed");    
    }
    root = parseSamlMessage(inflate(decodeBase64(parsed.samlMessage)));
} else {
    logger.info("Process SAML LogoutResponse with HTTP-POST binding");
    root = parseSamlMessage(decodeBase64(payload));
}

What is our expectation for how a WebApp should behave if it gets a LogoutResponse over a HTTP-POST binding that also has URL parameters?

I think the rule for Kibana is:

• If the http method is GET send the queryString to Elasticsearch, and ignore (or reject) any body (which is very unlikely to exist)
• if the http method is POST ignore (or reject) the query parameters, and send the body to Elasticsearch.

Is that our intent?

A good question. To be honest, I haven't really considered this thoroughly. I think your suggestion makes sense because the code proceeds as HTTP-POST, e.g. base64 encode, no deflate and XML signature, when content is present. This basically says content implies POST, which in turn means queryString implies GET.

I would personally prefer to ignore the body for GET and query string for POST since the spec does not explicitly forbid them.

Why do we have another class here, rather than just using SamlCompleteLogoutRequest ?

Because of copy/paste ... removed it. thanks